Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix multiple byte tests #31

Merged
merged 5 commits into from
Aug 14, 2021
Merged

Fix multiple byte tests #31

merged 5 commits into from
Aug 14, 2021

Conversation

seriousme
Copy link
Contributor

Byte is defined in the openapi specification as Base64
Base64 data can span multiple lines, hence the /gm.
The challenge with /gm is that it won't reset the lastIndex of the RegExp.
So when the next call tries to use the RegExp it will fail because lastIndex has not been reset.

This PR fixes that by defining the RegExp locally.
Added a test that proves it works ok now, that same test failed without the fix.

Kind regards,
Hans

@seriousme
Copy link
Contributor Author

Had to upgrade prettier to make CI work again ;-)

@coveralls
Copy link

Pull Request Test Coverage Report for Build 836689211

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.06%) to 92.704%
Files with Coverage Reduction New Missed Lines %
dist/formats.js 1 89.13%
Totals Coverage Status
Change from base Build 811386298: 0.06%
Covered Lines: 128
Relevant Lines: 135
💛 - Coveralls

@coveralls
Copy link

coveralls commented May 12, 2021 •
edited
Loading

Pull Request Test Coverage Report for Build 1130221971

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.09%) to 92.735%
Files with Coverage Reduction New Missed Lines %
dist/formats.js 1 89.21%
Totals Coverage Status
Change from base Build 1130183033: 0.09%
Covered Lines: 129
Relevant Lines: 136
💛 - Coveralls

@seriousme
Copy link
Contributor Author

@epoberezkin could you please review ?

Kind regards,
Hans

@epoberezkin
Copy link
Member

Had to upgrade prettier to make CI work again ;-)

prettier indeed made some change that changed the required formatting - annoying, I've been bitten by it too.

There are no changes in regexes, they are just on the next line, correct?

The challenge with /gm is that it won't reset the lastIndex of the RegExp.

Some quirk... Maybe some other method should be used, or is it just how it is?

@seriousme
Copy link
Contributor Author

seriousme commented May 14, 2021 •
edited
Loading

Had to upgrade prettier to make CI work again ;-)

prettier indeed made some change that changed the required formatting - annoying, I've been bitten by it too.

There are no changes in regexes, they are just on the next line, correct?

Correct, all other changes apart from byte are performed by prettier and just put them on the next line. I haven't touched them.

The challenge with /gm is that it won't reset the lastIndex of the RegExp.

Some quirk... Maybe some other method should be used, or is it just how it is?

It is the recommended way according to StackOverflow ;-)

Kind regards,
Hans

@tommy351 tommy351 mentioned this pull request May 20, 2021
Merged
@seriousme
Copy link
Contributor Author

@epoberezkin are you ok with this fix ?

@seriousme
Copy link
Contributor Author

@epoberezkin can you please review again ?

Kind regards,
Hans

epoberezkin
epoberezkin requested changes Aug 1, 2021
View reviewed changes
Copy link
Member

@epoberezkin epoberezkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, to reduce noise, I suggest fixing prettier version at 2.0.5 and I would commit style changes separately

src/formats.ts
function byte(str: string): boolean {
const BYTE = /^(?:[A-Za-z0-9+/]{4})*(?:[A-Za-z0-9+/]{2}==|[A-Za-z0-9+/]{3}=)?$/gm
return BYTE.test(str)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think rather than defining regex locally it would be better to reset the index on regex after the call and have regex itself defined once in the scope of this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To keep the code concise I propose to do it like this:

const BYTE = /^(?:[A-Za-z0-9+/]{4})*(?:[A-Za-z0-9+/]{2}==|[A-Za-z0-9+/]{3}=)?$/gm

function byte(str: string): boolean {
  BYTE.lastIndex = 0
  return BYTE.test(str)
}

Ok ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes - thank you

@seriousme
Copy link
Contributor Author

I tried downgrading my local prettier to 2.0.5 but that results in other problems:

> prettier --list-different "./**/*.{md,json,yaml,js,ts}"

src/index.ts[error] src/index.ts: SyntaxError: Expression expected. (55:27)
[error]   53 | 
[error]   54 | function addFormats(ajv: Ajv, list: FormatName[], fs: DefinedFormats, exportName: Name): void {
[error] > 55 |   ajv.opts.code.formats ??= _`require("ajv-formats/dist/formats").${exportName}`
[error]      |                           ^
[error]   56 |   for (const f of list) ajv.addFormat(f, fs[f])
[error]   57 | }
[error]   58 |

So can I propose that you upgrade prettier on master after which I will add my few lines of code?

Kind regards,
Hans

@epoberezkin
Copy link
Member

So can I propose that you upgrade prettier on master after which I will add my few lines of code?

yes - thank you - done!

@epoberezkin
Copy link
Member

@seriousme actually, the diff is now smaller, so the only thing that's left is taking regex out of the function - will you do it?

Thank you

@seriousme
Copy link
Contributor Author

@seriousme actually, the diff is now smaller, so the only thing that's left is taking regex out of the function - will you do it?

Thank you

Like I said, most of the changes were due to prettier ;-)
I have moved the regexp out of the function.

Please review again.

Kind regards,
Hans

@epoberezkin epoberezkin merged commit 10669ff into ajv-validator:master Aug 14, 2021
@seriousme seriousme deleted the fix-multiple-byte-tests branch August 14, 2021 09:35
@YoutacRandS-VA YoutacRandS-VA mentioned this pull request Jun 19, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@epoberezkin epoberezkin epoberezkin approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@seriousme @coveralls @epoberezkin